server.js: Refactor profile server code into own file#4656
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4656 +/- ##
=======================================
Coverage 88.32% 88.32%
=======================================
Files 300 300
Lines 26808 26808
Branches 7242 7241 -1
=======================================
Hits 23679 23679
Misses 2917 2917
Partials 212 212 ☔ View full report in Codecov by Sentry. |
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
Hey @kazarmy! I'm so sorry I didn't look at this earlier.
I believe this is solid work! But as you said yourself too, this looks a bit complicated compared to what we have already. I looked at it and tried to find ways to make it less complicated but I believe that's not really possible. Therefore I think that in this current form, this doesn't bring very useful improvements over the current situation.
I can think of another possibility: have a script that would only spawn a server serving a file + open a tab, but no launching the profiler UI.
I can imagine that this functionality would be in a importable file so that it can be used in the server.js (replacing some of that existing code) and could also be imported from a script.
This would simplify server.js (because the http server spawn would be in a different file) and provide the functionality of opening the profiler UI loading some local file even if the server is already launched.
I also believe this would be simpler because there wouldn't need to do all the complex work around temporary config files.
What do you think?
| ], | ||
| extensions: ['.js', '.jpg'], | ||
| }, | ||
| node: true, |
There was a problem hiding this comment.
What error did you get without that? I don't see an error when I remove this locally.
There was a problem hiding this comment.
ah I know, yarn lint-js doesn't look at *.mjs but just *.js... So this file wasn't checked.
There was a problem hiding this comment.
maybe it should have ... I haven't yet found a good CommonJS equivalent to top-level await.
This reverts commit 91b485c.
…nto separate-launch-fp-process
|
|
kazarmy
changed the title
from-url URL in separate script
|
Progress on this PR is a tad unexpected but thanks for the compliment and review! Hopefully I haven't forgotten significant amounts of JavaScript in the meantime.
Not sure what you mean by "open a tab", but if you're implying that the profile server should eventually be a gateway to multiple local profile files, that would probably require some more thinking in a different PR.
Assuming that I have interpreted your words correctly, I'm ok with simplifications like 24c73d5 (not sure whether a class is needed here) as long as people are happy with it even without an immediate follow-up. |
…nto separate-launch-fp-process
Sorry for not being super clear. I meant:
(I'm writing this comment before looking at your code, doing it now...) |
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
This looks good to me, we can merge it this way and then maybe somebody else can take it from there :) I can imagine you'd like to move on !
| )}`; | ||
| // Start profile server and open on profile. | ||
| profileServer.start(host, profilerUrl, argv.profile, (profileFromUrl) => { | ||
| import('open').then((open) => open.default(profileFromUrl, openOptions)); |
There was a problem hiding this comment.
The open part could go to the utility function too. The utility could be called serveAndOpen
There was a problem hiding this comment.
Apart from the dual responsibility, this makes sense and it doesn't seem reasonable to punt this to a follow-up salami slice PR so f9ce3fd
|
Just waiting for your answer before we merge it! |
…nto separate-launch-fp-process
Oh ok yeah I can miss the obvious so being super clear is great! I'll try so that being super clear isn't required though.
I suppose you have to reapprove now? |
|
Thanks again for your patience! |
2 participants
As per #4452 (review), this PR pulls out the profile server code in
server.jsinto a separate script that spawns a local profiler server viaserver.jsand opens the profiler with afrom-urlURL. Yes, it's definitely more interesting, though things get more complicated when one can't share information through the same address space. Not sure whether the code can't be simplified.Some notes:
.mjsdue to the conveniences of top-levelawaitand easy importing of theopenpackage..eslintrc.jsis for ESLint to recognizenode:httpas a valid Node module.